Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PayPal API Show Total #597

Closed
wants to merge 1 commit into from
Closed

Conversation

tomshaw
Copy link
Contributor

@tomshaw tomshaw commented Nov 19, 2023

This update converts the show totals property to string value true or false.

This update converts the show totals property to string value true or false.
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@srmklive
Copy link
Owner

I don't think its needed here.

@srmklive srmklive closed this Nov 21, 2023
@tomshaw
Copy link
Contributor Author

tomshaw commented Nov 21, 2023

Actually it is need there. PayPal has no idea what total_required=1 is. Should be total_required=true.

List invoices

$this->paypal->getProvider()->setPageSize($this->pageSize)->setCurrentPage($this->currentPage)->showTotals(true);

@srmklive
Copy link
Owner

srmklive commented Nov 21, 2023

I would not do it like this. There are also other API calls where it is needed. If you can update PR with a fix that can be used across the package, that would be great.

@srmklive srmklive reopened this Nov 21, 2023
@tomshaw
Copy link
Contributor Author

tomshaw commented Nov 22, 2023

Absolutely. Completely agree repeating the same line of code is anti pattern. Being unfamiliar with the code base I wonder what a satisfactory solution might be? A URL builder class or helper function? Will take a look tomorrow.

srmklive added a commit that referenced this pull request Nov 22, 2023
@srmklive
Copy link
Owner

@tomshaw look at the referenced commit. I think this resolves the issue.

@tomshaw tomshaw closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants